Support async_sequence - #2533
Conversation
|
Please just do the async_sequence part, the other parts are too big to consume. |
Sure, but how can we test it without upgrading the library? |
|
That's a good question. But it's kinda more important to make the job pass than making it correct right now, we'll get it checked later when we get the actual IDL updated. |
Signed-off-by: Bashamega <adambashaahmednaji@gmail.com>
Done. Thanks for looking at this. When will you check my previous PRs? Or should I close them? |
| } | ||
| return "AsyncIterable"; | ||
| } else { | ||
| // Legacy main & fallback builds: no async iterable interface available. |
There was a problem hiding this comment.
What is main & fallback builds? Can we say legacy TS versions?
| if (compilerBehavior.treatAsyncSequence) { | ||
| // Forwards-compatible definition: | ||
| // NOTE: In IDL→TS (whatwg/streams#1372 etc) async_sequence<T> -> AsyncIterable<T> | ||
| // If stricter interop wanted, could use AsyncIterable<T> | Iterable<T> |
There was a problem hiding this comment.
Why?
| // For TS <2.3, the generator separates out [Symbol.asyncIterator] into *.asynciterable.d.ts, so | ||
| // these references must not leak into the main file, so we can treat them as `any` as a fallback. | ||
| if (compilerBehavior.treatAsyncSequence) { | ||
| // Forwards-compatible definition: |
There was a problem hiding this comment.
forwards-compatible?
| // Support async_sequence (see https://github.com/whatwg/streams/pull/1372) | ||
| if (obj.type === "async_sequence") { | ||
| // If we're in a build that includes async iterables (TS 2.3+ or forced), emit as AsyncIterable<T>. | ||
| // For TS <2.3, the generator separates out [Symbol.asyncIterator] into *.asynciterable.d.ts, so |
There was a problem hiding this comment.
I don't think we care about TS < 2.3, minimum supported version is already 4.4.
That said, AsyncIterable does require separate asynciterable.d.ts until TS 6.0, so to make it correct we need to defer any functions that uses async_sequence to asynciterable variant.
Given we don't really test it here, it's also acceptable to just entirely skip the generation for those functions with some console warning.
There was a problem hiding this comment.
I don't think we are doing this?
There was a problem hiding this comment.
I don't quite understand this comment but i have pushed a change tell me if this is what you mean
| outputFolder: new URL("./ts5.5/", outputFolder), | ||
| compilerBehavior: {}, // ts5.5 does not support `IteratorObject` or unrelated setter types | ||
| compilerBehavior: { | ||
| treatAsyncSequence: false, |
There was a problem hiding this comment.
Let it just empty as others did here
|
done |
… warn for TypeScript versions below 6.0
| compilerBehavior: { | ||
| useIteratorObject: true, | ||
| allowUnrelatedSetterType: true, | ||
| treatAsyncSequence: false, |
There was a problem hiding this comment.
Again, please skip it as others do
There was a problem hiding this comment.
Sorry, done
| const type = convertBaseType(); | ||
| if (type === "any") { | ||
| return type; | ||
| } |
There was a problem hiding this comment.
Does this still matter?
There was a problem hiding this comment.
Updated
…ine type conversion process
|
Thanks! For future reference - I think the fix is in a wrong place - Ideally it should be somewhere in emitMethod or the caller of emitMethod, check the return type from there, and skip the emission for older TS versions. We should not emit But for now this is okay as we'll check it later. LGTM |
|
Merging because Kagami Sascha Rosylight (@saschanaz) is a code-owner of all the changes - thanks! |
|
As for future work - I think it would be nice to consume each of webref/idl and BCD updates release-by-release to reduce #2495 and eventually close it. About KDL work - I have been thinking of having a library - https://codeberg.org/krosylight/kdl2json, but hasn't been made any progress recently. I was thinking maybe I should encourage contribution once I get it more in form, but any help (or any question for future direction to contribute for) would be nice. |
Hello, |
closes #2480